[sram_ctrl,dv] Update coverage exclusions #25713
Conversation
|
Before this PR: |
nasahlpa
requested review from
eshapira,
rswarbrick and
vogelpi
and removed request for
a team and
eshapira
|
Now, things from sram_ctrl_cov_excl.el landed in sram_ctrl_unr_excl.el. Is this OK? What should be in which file? |
| @@ -3,149 +3,137 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| //================================================== | |||
| // This file contains the Excluded objects | |||
| // Generated By User: root | |||
| // Generated By User: nasahlpa | |||
There was a problem hiding this comment.
This is manually generated, right? It seems perfectly sensible but maybe we should either change the filename to stop it looking like it might be automatic or at least add a comment at the top that explains what's going on?
I'd also consider pulling the items with non-trivial explanations into a separate list at the top of the file to make it more obvious that we've reasoned about them.
There was a problem hiding this comment.
Yes, exactly - I've created this list manually.
Should I move this into the sram_ctrl_cov_excl.el file or should I create a new one?
Good idea, I will move the more complex exclusions to the top of the file.
There was a problem hiding this comment.
I think I'd be inclined to move the non-trivial exclusions into their own file. That way, we don't end up getting stomped on when we next run UNR properly.
| Block 23 "264295034" "rdata_o[k] = rdata[k];" | ||
| CHECKSUM: "4224194069 2881513198" | ||
| CHECKSUM: "3523621980" | ||
| ANNOTATION: "[UNR] all inputs are constant" |
There was a problem hiding this comment.
I believe this, but maybe it makes sense to expand the explanation slightly. Here, maybe something like "This is a continuous assignment in a buffer whose arguments are parameters" ?
| INSTANCE:tb.dut.u_seed_anchor.u_secure_anchor_buf.gen_generic.u_impl_generic | ||
| CHECKSUM: "2099741489 2073313596" | ||
| INSTANCE: tb.dut.u_reg_regs.u_status_bus_integ_error.wr_en_data_arb | ||
| ANNOTATION: "[UNR] all inputs are constant" |
There was a problem hiding this comment.
This probably needs explaining a bit more (because it's manual). I think that d here is the d that gets passed into prim_subreg, which is hw2reg.status.bus_integ_error.d and that is a constant '1 (the error gets signalled by turning on the write-enable).
The same argument applies to the next two items.
| INSTANCE: tb.dut.u_tlul_lc_gate | ||
| Fsm state_q "3443824386" | ||
| ANNOTATION: "VC_COV_UNR" | ||
| State StFlush "76" | ||
| CHECKSUM: "74367784 3785313510" | ||
| INSTANCE: tb.dut.u_reg_regs.u_reg_if | ||
| Fsm state_q "3443824386" | ||
| ANNOTATION: "VC_COV_UNR" |
There was a problem hiding this comment.
I think this might need an explicit bit of reasoning? (I can't see the flush->active transition in the old UNR)
There was a problem hiding this comment.
This exclusion is already here in the sram_ctrl_cov_excl.el file. I will update the description.
| Transition StFlush->StActive "76->289" | ||
| CHECKSUM: "704952876 1147758610" | ||
| INSTANCE: tb.dut.u_prim_sync_reqack_data.u_prim_sync_reqack | ||
| ANNOTATION: "[UNSUPPORTED] ACK can't come without REQ" |
There was a problem hiding this comment.
The RTL isn't super-obvious to me. I believe this might be true. But can you explain why? (There's an FSM involved that needs a little bit of reasoning about, I think)
There was a problem hiding this comment.
This exclusion is already present here in the existing sram_ctrL-cov_excl file.
| Condition 1 "1414883863" "(src_req_i & src_ack_o) 1 -1" | ||
| CHECKSUM: "998580748 639524789" | ||
| INSTANCE: tb.dut.u_tlul_lc_gate | ||
| ANNOTATION: "[LOWRISK] This happens in the 1st cycle after exiting reset. In order to cover it, need to drive TL items during reset, which isn't supported in the agent." |
There was a problem hiding this comment.
An appropriate tag, given our employer :-)
I'm not completely sure about the explanation. Instead of "reset", do you mean something lc_en_i? I think this might be possible (but I'm willing to believe that we don't care!)
There was a problem hiding this comment.
This exclusion is already present here in the existing sram_ctrL-cov_excl file.
As recently two new registers (status_readback_error and status_sram_alert) were added, this commit adds two new cov exclusions that are already present for the other error registers. These exclusions are needed as the wr_data signal is directly tied to 1'b1. Furthermore, this commit adds a more detailed explanation why these exclusions are needed and moves them to the upper part of the file to indicate that they were manually added. Finally, these exclusions were added to a new file to cleary mark them as manually added. Signed-off-by: Pascal Nasahl <[email protected]>
As the depth of the FIFO is one, we can exclude a couple of uncovered conditions. Signed-off-by: Pascal Nasahl <[email protected]>
This commit manually updates an automatically generated coverage exclusion as it is outdated. In the next UNR run this can be overwritten but it helps for now to reach V3. Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa
force-pushed
the
sram_update_exclusions
branch
from
07627dc to
9e6c316
Compare
|
Thanks for your review, @rswarbrick! I've simplified and restructured this PR - could you please again take a look? |
This commit excludes the default branch of the FSM inside the tlul_adapter_sram_byte module as we should not reach it under normal operational conditions. Signed-off-by: Pascal Nasahl <[email protected]>
nasahlpa
force-pushed
the
sram_update_exclusions
branch
from
9e6c316 to
5436b87
Compare
johngt
changed the title
This PR consists of three different commits: